Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Send one full update from y-websocket #6650

Merged
merged 16 commits into from
Nov 26, 2024
Merged

Send one full update from y-websocket #6650

merged 16 commits into from
Nov 26, 2024

Conversation

max-nextcloud
Copy link
Collaborator

@max-nextcloud max-nextcloud commented Nov 14, 2024 •
edited
Loading

Always send full diff to server state.

  • Inline y-websocket, so we can adjust it to our needs.
  • Track the server state by updating a ydoc with all the updates
    received from the server.
  • Compute updates based on that server state.
  • Only send the last update from the queue (as it includes all previous).

Todo:

  • Check why it does not work with document state provided.
  • Confirm it works for new files with content.

@max-nextcloud max-nextcloud requested a review from mejo- as a code owner November 14, 2024 13:50
@max-nextcloud max-nextcloud marked this pull request as draft November 14, 2024 13:54
@max-nextcloud max-nextcloud force-pushed the fix/y-websocket-with-full-updates branch from 2cfb371 to 4675077 Compare November 14, 2024 13:55
@juliusknorr juliusknorr added this to the Nextcloud 31 milestone Nov 14, 2024
@max-nextcloud max-nextcloud self-assigned this Nov 14, 2024
@max-nextcloud max-nextcloud force-pushed the fix/y-websocket-with-full-updates branch 4 times, most recently from 1495034 to f08a168 Compare November 18, 2024 11:01
@max-nextcloud max-nextcloud marked this pull request as ready for review November 18, 2024 11:32
juliusknorr
juliusknorr reviewed Nov 18, 2024
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave this a first try and basic syncing is still working fine, also some offline online switching on two browser sessions worked as expected.

One small difference I noticed (without reading the code yet in detail):

  • When loading the file it seems we no longer wait until the content is loaded to hide the loading indicator, we start with an empty document and can type before any document state is propagated to us. @max-nextcloud is this expected or can it cause any side effects?

max-nextcloud
max-nextcloud commented Nov 19, 2024
View reviewed changes
lib/Service/DocumentService.php Outdated
$getStepsSinceVersion = count($querySteps) > 0 ? 0 : $version;
// If there were any queries in the steps send all steps since last save.
$getStepsSinceVersion = count($querySteps) > 0
? $document->getLastSavedVersion()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be min($document->getLastSavedVersion(), $version) to make sure we don't skip any steps

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in the next commit by including the document state that matches $document->getLastSavedVersion

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still using the smaller would mean we have a redundancy in case the client that did the save was out of sync.

@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Nov 21, 2024 •
edited
Loading

What's missing:

  • license for y-websocket
  • remove $queue from Editor.vue
  • loading spinner
  • ensure to only store remote state in documentState.

@max-nextcloud max-nextcloud force-pushed the fix/y-websocket-with-full-updates branch from f08a168 to e786a18 Compare November 25, 2024 09:34
@max-nextcloud
fix(sync): inline y-websocket to track received updates there
c1c4d38 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(lint): y-websocket
e78350a 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
enh(y-websocket): always send full diff to server state
6613cf6 
Keep an internal ydoc tracking updates that came from the server.
Send updates, that would sync this doc with the current doc state.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(license): add license header and props to y-websocket
68bd8e3 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(awareness): only send updates about the local client
ed57736 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(sync): make use of steps in push responses
f2da61f 
The pushed steps are echoed back with all other steps since version immediately.
Processing them reduces the size of the following pushes and syncs.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
refactor(sync): rename _receiveSteps to receiveSteps
6f66f30 
It has always been publicly called from the PollingBackend.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
refactor(sync): drop superfluous update message
5fa9517 
Updates now include all the local structs
that were not yet received from remote.

No need to compute a separate update message anymore.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(sync): Do not resend document state in first push.
ec059cc 
Apply document state as a step.
Process it like other steps received from the remote.
In particular include it in the tracking of steps already applied
and set the version accordingly.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(sync): reply to queries with steps since last save
0173b9d 
This was a very inefficient attempt to resync
that we did not even process on the client side.

Only the steps since the last save may not be enough
to get back in sync.
However we can expand this by including the document state
or storing it as the first step after a save.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(sync): include document state in response to queries
0a32e42 
Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
fix(sync): process document state from push response
3bc7015 
Do not process document state from create response.
During create the editor has not been initialized fully
and the cursor position is 0 - which is invalid
as it is not inside a node with inline content.
(It is inside the doc before the initial paragraph.)

This also allows processing document state later on
in order to recover from out of sync situations.
But we do not make use of that yet.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud max-nextcloud force-pushed the fix/y-websocket-with-full-updates branch 3 times, most recently from ea1a4fb to 91450a2 Compare November 26, 2024 09:23
mejo-
mejo- reviewed Nov 26, 2024
View reviewed changes
Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments.

src/services/SyncService.js Outdated Show resolved Hide resolved
src/services/Outbox.ts Outdated Show resolved Hide resolved
src/services/Outbox.ts Outdated Show resolved Hide resolved
@max-nextcloud
Copy link
Collaborator Author

  • When loading the file it seems we no longer wait until the content is loaded to hide the loading indicator, we start with an empty document and can type before any document state is propagated to us. @max-nextcloud is this expected or can it cause any side effects?

I cannot reproduce this right now.

@max-nextcloud max-nextcloud force-pushed the fix/y-websocket-with-full-updates branch 2 times, most recently from c0b1bd6 to 3bc7015 Compare November 26, 2024 11:00
max-nextcloud and others added 4 commits November 26, 2024 12:44
@max-nextcloud
refactor(sync): cache steps in sync service
49a9705 
Store the steps that need to be send
where we also do the debouncing.

They will be updated whenever there is a new message from y-websocket.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
refactor(Editor): instantiate api outside sync service
7396ce9 
Signed-off-by: Max <max@nextcloud.com>
@mejo-
test(cypress): Pass SessionApi into the sync service
d1b6741 
Signed-off-by: Jonas <jonas@freesources.org>
@max-nextcloud
fix(SyncService): only log errors for now
511b304 
Signed-off-by: Max <max@nextcloud.com>
@mejo- mejo- merged commit 1c25d7e into main Nov 26, 2024
66 checks passed
@mejo- mejo- deleted the fix/y-websocket-with-full-updates branch November 26, 2024 14:46
@mejo-
Copy link
Member

mejo- commented Nov 26, 2024

/backport to stable30

@mejo-
Copy link
Member

mejo- commented Nov 26, 2024

/backport to stable29

@backportbot backportbot bot mentioned this pull request Nov 26, 2024
Merged
@backportbot backportbot bot mentioned this pull request Nov 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mejo- mejo- mejo- left review comments

@juliusknorr juliusknorr juliusknorr approved these changes

Assignees

@max-nextcloud max-nextcloud

Labels
3. to review bug Something isn't working feature: sync
Projects
📝 Office team
Status: ☑️ Done
Milestone
Nextcloud 31
Development

Successfully merging this pull request may close these issues.
3 participants
@max-nextcloud @mejo- @juliusknorr